#397 : "More from this model" image preview section #408
Conversation
There was a problem hiding this comment.
Hi @konarshankar07 thanks for the pull request! Please take a look at the review comments
| /* | ||
| * Series ID. | ||
| */ | ||
| const SERIE_ID = 'serie_id'; |
There was a problem hiding this comment.
To avoid code duplication, it's better to use the field as a method parameter or as a constructor parameter provided by DI and use cycles instead of similar code blocks
There was a problem hiding this comment.
Hello @sivaschenko ,
We can avoid code duplication by replacing Magento\Framework\Api\Search\SearchCriteriaBuilder to Magento\Framework\Api\SearchCriteriaBuilder so that we can pass array of filter but we have already used Magento\Framework\Api\Search\SearchCriteriaBuilder in so many places that there are lot of chance some functionality might broke. Let me know if you want to move forward with this logic or any other logic from your side is always welcome
Thanks!
| /** | ||
| * Returns series to display under the image | ||
| * | ||
| * @param record | ||
| * @returns {*[]} | ||
| */ | ||
| getSeries: function (record) { | ||
| if (!record.series) { | ||
| if (!record.series || !record.model) { |
There was a problem hiding this comment.
it's better to process series and same model images separately (i.e. use a loop)
…into 397-more-from-this-model
There was a problem hiding this comment.
Hi @konarshankar07 thanks for updates.
Please take a look at my commit with removed code duplication and see a couple of additional comments in this code review.
Also please pay attention that GetImageSeriesTest has to be adjusted
| <!-- ko foreach: $col.getSeries($row()) --> | ||
| <div class="thumbnail" data-bind="css: { 'hide': $index() >= 3}"> | ||
| <img attr="src: thumbnail_url, alt: title"> | ||
| <div id="adobe-stock-tabs" attr="'data-mage-init': getDataMageInit()"> |
There was a problem hiding this comment.
That is incorrect usage of data-mage-init attribute. I think data-mage-init attribute should be static in this case.
| <div id="adobe-stock-tabs" attr="'data-mage-init': getDataMageInit()"> | ||
| <ul class="tabs-horiz"> | ||
| <li> | ||
| <a href="#series_content" id="series_tab"> |
There was a problem hiding this comment.
As there are multiple tabs per page, I'd suggest using unique ids (i.e. appending image id or index)
| $this->getImageList->execute($serieSearchCriteria)->getItems(), | ||
| $this->getImageList->execute($modelSearchCriteria)->getItems() | ||
| ); | ||
| foreach ($this->fields as $key => $field) { |
There was a problem hiding this comment.
We can still refactor the code using the filter group for better searching. Please see devdocs for more information in order to do that we need to change SearchCriteriaBuilder interface which I mention in the previous comment. Correct me if I'm wrong.
Thanks
There was a problem hiding this comment.
Thanks for the suggestion! Do you mean we might have been able to add both filters to a single filter group and perform a single request? I don't think that will be possible considering that it's required to keep the related image groups separate.
There was a problem hiding this comment.
Yes, I think you are right. I tried to implement and failed to achieve the expected result
|
Hello @sivaschenko , |
|
Hello @sivaschenko , |
|
@konarshankar07 here is the file https://github.com/magento/adobe-stock-integration/blob/develop/AdobeStockImage/Test/Unit/Model/GetImageSeriesTest.php That test is causing build failure and has to be corrected |
|
Hello @sivaschenko , Thanks |
|
Hi @konarshankar07 I believe that is because of conflicts |
Description (*)
This PR is regarding addition of more from this model section in the image preview
Fixed Issues (if relevant)
Manual testing scenarios (*)
Actual Result